Skip to content

fix: Change login in field.setValue to use optional operator #12909

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Jan 27, 2025
Merged

fix: Change login in field.setValue to use optional operator #12909

merged 9 commits into from
Jan 27, 2025

Conversation

smilingkylan
Copy link
Contributor

@smilingkylan smilingkylan commented Jan 8, 2025 •
edited
Loading

Description

Login view attempts to reset the password input field value but sometimes the ref for the input field is null. Therefore, I have turned fieldRef.setValue to fieldRef?.setValue. Alternatively, since this occurs once successful navigation occurs and component is about to unmount, we may also be able to just remove the line.

Related issues

Addresses this Sentry issue: https://metamask.sentry.io/share/issue/ef784f4e764a4a76bc505dcdaa52c0ec/
GitHub issue: #12653

Manual testing steps

  1. You can manually set the ref to null in the code and login to the app. It should not trigger the catch clause right underneath it.

Screenshots/Recordings

n/a but can producer if necessary

Before

n/a

After

login-setvalue.mov

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@smilingkylan smilingkylan added the team-mobile-platform Mobile Platform team label Jan 8, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 8, 2025

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@smilingkylan smilingkylan added the Run Smoke E2E Requires smoke E2E testing label Jan 8, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 8, 2025 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 8d2c748
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/96f277d7-5744-4506-8687-0cf60abdebed

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@smilingkylan smilingkylan changed the title [Bug] Change login in field.setValue to use optional operator fix: Change login in field.setValue to use optional operator Jan 8, 2025
@smilingkylan smilingkylan added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-mobile-platform Mobile Platform team and removed team-mobile-platform Mobile Platform team labels Jan 8, 2025
@smilingkylan smilingkylan marked this pull request as ready for review January 10, 2025 16:34
@smilingkylan smilingkylan requested a review from a team as a code owner January 10, 2025 16:34
@sethkfman sethkfman linked an issue Jan 13, 2025 that may be closed by this pull request
[Sentry] TypeError: Cannot read property 'setValue' of null #12653
Closed
Cal-L
Cal-L requested changes Jan 15, 2025
View reviewed changes
Copy link
Contributor

@Cal-L Cal-L left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this prevents the crash, it doesn't solve the issue of the ref not being assigned correctly. Referencing this thread - https://consensys.slack.com/archives/C081B61CDS9/p1734459919055509?thread_ts=1734414724.387639&cid=C081B61CDS9, I suggested that we need to forward the ref to the TextInput so that the ref is propagated through. See implementation here - https://github.com/MetaMask/metamask-mobile/pull/12649/files#diff-b7bdacbc89185049ac31d9f4ede89235bf33402b7c5ea3977fc6328c11d51d06R22

@smilingkylan smilingkylan requested a review from a team as a code owner January 15, 2025 23:36
smilingkylan
smilingkylan commented Jan 15, 2025
View reviewed changes
smilingkylan
smilingkylan commented Jan 15, 2025
View reviewed changes
Cal-L
Cal-L reviewed Jan 16, 2025
View reviewed changes
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a comment

@smilingkylan smilingkylan added Run Smoke E2E Requires smoke E2E testing and removed Run Smoke E2E Requires smoke E2E testing labels Jan 16, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 16, 2025 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: f08c19a
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/30f3b322-9d28-4ec3-add1-63251583eea6

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

brianacnguyen
brianacnguyen requested changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@brianacnguyen brianacnguyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you attach a video of current usage of the TextField/Input still being functional?

@smilingkylan
Copy link
Contributor Author

Can you attach a video of current usage of the TextField/Input still being functional?

Done

@smilingkylan smilingkylan requested a review from a team as a code owner January 22, 2025 20:17
@smilingkylan smilingkylan removed the Run Smoke E2E Requires smoke E2E testing label Jan 22, 2025
@smilingkylan
Copy link
Contributor Author

Please see this Slack thread for justification of removal of autoFocus for Login password input field

@smilingkylan smilingkylan added Run Smoke E2E Requires smoke E2E testing and removed Run Smoke E2E Requires smoke E2E testing labels Jan 23, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 23, 2025 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 6a76363
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/3d16c5e0-9dda-4052-9a20-ce50c7b0d6ee

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
50.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@smilingkylan smilingkylan added Run Smoke E2E Requires smoke E2E testing and removed Run Smoke E2E Requires smoke E2E testing labels Jan 23, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 23, 2025 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 1416d7e
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/82827be6-1e37-4d51-a3fa-211cf8bb750c

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@smilingkylan smilingkylan added Run Smoke E2E Requires smoke E2E testing and removed Run Smoke E2E Requires smoke E2E testing labels Jan 24, 2025
Cal-L
Cal-L approved these changes Jan 27, 2025
View reviewed changes
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@smilingkylan smilingkylan added this pull request to the merge queue Jan 27, 2025
Merged via the queue into main with commit 7a424b9 Jan 27, 2025
54 checks passed
@smilingkylan smilingkylan deleted the bug/login-setValue branch January 27, 2025 21:45
@github-actions github-actions bot locked and limited conversation to collaborators Jan 27, 2025
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jan 27, 2025
@metamaskbot metamaskbot added the release-7.40.0 Issue or pull request that will be included in release 7.40.0 label Jan 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Cal-L Cal-L Cal-L approved these changes

@brianacnguyen brianacnguyen brianacnguyen approved these changes

@vinnyhoward vinnyhoward vinnyhoward approved these changes

@frankvonhoven frankvonhoven frankvonhoven approved these changes

Assignees

@smilingkylan smilingkylan

Labels
release-7.40.0 Issue or pull request that will be included in release 7.40.0 Run Smoke E2E Requires smoke E2E testing team-mobile-platform Mobile Platform team
Projects
PR review queue
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Sentry] TypeError: Cannot read property 'setValue' of null
6 participants
@smilingkylan @Cal-L @brianacnguyen @vinnyhoward @frankvonhoven @metamaskbot